HBASE-25249 Adding StoreContext #2800
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
apurtell
requested changes
Dec 21, 2020
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java
Outdated
Show resolved
Hide resolved
| @@ -2559,7 +2572,7 @@ public boolean needsCompaction() { | |||
| * @return cache configuration for this Store. | |||
| */ | |||
| public CacheConfig getCacheConfig() { | |||
| return this.cacheConf; | |||
| return storeContext.getCacheConf(); | |||
| } | |||
|
|
|||
| public static final long FIXED_OVERHEAD = ClassSize.estimateBase(HStore.class, false); | |||
There was a problem hiding this comment.
Does DEEP_OVERHEAD and heapSize() need to change? Does TestHeapSize still pass?
There was a problem hiding this comment.
TestHeapSize still passed when I checked it locally.
from what I understood from the FIXED_OVERHEAD were being calculated by ClassSize.estimateBase, it will automatically calculate the our newly added fields and reference included the change of HStoreContext storeContext.
So for the DEEP_OVERHEAD that takes new calculated FIXED_OVERHEAD to come up the heap size, it should be done already. Or did I miss something here?
There was a problem hiding this comment.
this has been change as well, can you have another look ?
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStoreContext.java
Outdated
Show resolved
Hide resolved
|
@apurtell requested changes updated. I shouldn't rebase and squash for the updated changes, will be providing a new diff on top of the first/original change next time |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
saintstack
reviewed
Dec 23, 2020
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreUtils.java
Outdated
Show resolved
Hide resolved
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java
Outdated
Show resolved
Hide resolved
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
Adding HStoreContext which contains the metadata about the HStore. This meta data can be used across the HFileWriter/Readers and other HStore consumers without the need of passing around the complete store and exposing its internals.
- rename HStoreContext to StoreContext - rewrite the getChecksumType - fix one more style issues
z-york
reviewed
Dec 23, 2020
| @@ -246,6 +234,8 @@ | |||
| private AtomicLong compactedCellsSize = new AtomicLong(); | |||
| private AtomicLong majorCompactedCellsSize = new AtomicLong(); | |||
|
|
|||
| private StoreContext storeContext; | |||
There was a problem hiding this comment.
Should this be final? I wouldn't expect you would need to change StoreContext (the reference) after it has been initialized
There was a problem hiding this comment.
you're right, it could be final, will do it in next commit
| .withCompressTags(family.isCompressTags()) | ||
| .withChecksumType(checksumType) | ||
| .withBytesPerCheckSum(bytesPerChecksum) | ||
| .withCompressTags(getColumnFamilyDescriptor().isCompressTags()) |
There was a problem hiding this comment.
Does it simplify things to be able to pass StoreContext directly to the builders?
There was a problem hiding this comment.
I'd like to keep the current way that passing required fields to the creation of HFileContext. Mainly HFileContext is also a member of the StoreContext which if we pass into StoreContext to the creation of HFileContext, it will be a loop and is very strange.
|
🎊 +1 overall
This message was automatically generated. |
taklwu
changed the title
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
saintstack
reviewed
Jan 5, 2021
| public void setCompression(Compression.Algorithm compressAlgo) { | ||
| this.compressAlgo = compressAlgo; | ||
| } | ||
|
|
There was a problem hiding this comment.
Whats going on here?
HFileContext is supposed to be ' Read-only HFile Context Information' but here we are adding a setter. I see there are one or two setters but we also have HFileContextBuilder. Shouldn't we be going via the Builder making HFileContexts? And why is this method not added on the Builder? (Can it be added non-public to encourage users to go via the Builder)?
There was a problem hiding this comment.
fixed, and reverted back to builder pattern
There was a problem hiding this comment.
You haven't uploaded new PR so will leave this as unresovled for now.
There was a problem hiding this comment.
oops, I missed this change. and now it should have updated/removed.
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java
Outdated
Show resolved
Hide resolved
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java
Outdated
Show resolved
Hide resolved
| return this.storeContext.getFamily(); | ||
| } | ||
|
|
||
| public Encryption.Context getEncryptionContext() { |
There was a problem hiding this comment.
Has to be public? Has to be on HStore? Can it be on StoreContext? Is there a getStoreContext method on HStore?
There was a problem hiding this comment.
getEncryptionContext was used by HMobStore such we set it to public , it could be package-private.
getStoreContext was planned to add in a followup PR when used, tho you're right that if we have a getStoreContext then this getEncryptionContext could be removed
There was a problem hiding this comment.
Package private sounds good if only used locally.
| fileContext.setIncludesMvcc(includeMVCCReadpoint); | ||
| fileContext.setIncludesTags(includesTag); | ||
| fileContext.setCompression(compression); | ||
| fileContext.setFileCreateTime(EnvironmentEdgeManager.currentTime()); |
There was a problem hiding this comment.
Why would we do this here and not as methods on the builder?
There was a problem hiding this comment.
it was trying to reuse the defaultFileContext built with initializeStoreContext, but after rethinking on your comment, I'm going to remove the defaultFileContext and go back with the original builder pattern.
There was a problem hiding this comment.
That'd be better. If a builder, lets use it everywhere (can add a constructor on builder that allows a bit of shortcutting auto-filling most fields....)
| /** | ||
| * This carries the information on some of the meta data about the HStore. This | ||
| * meta data can be used across the HFileWriter/Readers and other HStore consumers without the | ||
| * need of passing around the complete store. |
There was a problem hiding this comment.
I would like to know here in class comment if this is read-only/immutable. I think it should be.
There was a problem hiding this comment.
Also, just a comment... the HFileContext has this for a comment...
- Read-only HFile Context Information. Meta data that is used by HFileWriter/Readers and by
- HFileBlocks. Create one using the {@link HFileContextBuilder} (See HFileInfo and the HFile
- Trailer class).
so it is for readers and writers.... So, StoreContext is probably fine but if you are wondering... here is incidences of Info.java vs Context.java.... If it helps.
kalashnikov:hbase.apache.git stack$ find src/main/java -name *Info.java
find: src/main/java: No such file or directory
kalashnikov:hbase.apache.git stack$ find hbase-*/src/main/java -name *Info.java
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/BackupTableInfo.java
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/BackupInfo.java
hbase-client/src/main/java/org/apache/hadoop/hbase/security/SecurityInfo.java
hbase-client/src/main/java/org/apache/hadoop/hbase/client/MutableRegionInfo.java
hbase-client/src/main/java/org/apache/hadoop/hbase/client/RegionInfo.java
hbase-common/src/main/java/org/apache/hadoop/hbase/util/VersionInfo.java
hbase-common/src/main/java/org/apache/hadoop/hbase/rsgroup/RSGroupInfo.java
hbase-hbtop/src/main/java/org/apache/hadoop/hbase/hbtop/mode/DrillDownInfo.java
hbase-hbtop/src/main/java/org/apache/hadoop/hbase/hbtop/field/FieldInfo.java
hbase-metrics-api/src/main/java/org/apache/hadoop/hbase/metrics/MetricRegistryInfo.java
hbase-replication/src/main/java/org/apache/hadoop/hbase/replication/ReplicationQueueInfo.java
hbase-server/src/main/java/org/apache/hadoop/hbase/snapshot/SnapshotInfo.java
hbase-server/src/main/java/org/apache/hadoop/hbase/util/HbckTableInfo.java
hbase-server/src/main/java/org/apache/hadoop/hbase/util/HbckRegionInfo.java
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileInfo.java
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/BlockWithScanInfo.java
hbase-server/src/main/java/org/apache/hadoop/hbase/namespace/NamespaceTableAndRegionInfo.java
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/ScanInfo.java
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFileInfo.java
hbase-server/src/main/java/org/apache/hadoop/hbase/master/webapp/RegionReplicaInfo.java
hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/CallQueueInfo.java
hbase-thrift/src/main/java/org/apache/hadoop/hbase/thrift2/generated/THRegionInfo.java
hbase-thrift/src/main/java/org/apache/hadoop/hbase/thrift/generated/TRegionInfo.java
kalashnikov:hbase.apache.git stack$ find hbase-*/src/main/java -name *Context.java
hbase-common/src/main/java/org/apache/hadoop/hbase/io/crypto/Context.java
hbase-common/src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDecodingContext.java
hbase-common/src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockEncodingContext.java
hbase-common/src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDefaultDecodingContext.java
hbase-common/src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDefaultEncodingContext.java
hbase-common/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileContext.java
hbase-common/src/main/java/org/apache/hadoop/hbase/io/TagCompressionContext.java
hbase-server/src/main/java/org/apache/hadoop/hbase/util/RowPrefixFixedLengthBloomContext.java
hbase-server/src/main/java/org/apache/hadoop/hbase/util/RowBloomContext.java
hbase-server/src/main/java/org/apache/hadoop/hbase/util/RowColBloomContext.java
hbase-server/src/main/java/org/apache/hadoop/hbase/util/BloomContext.java
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/ReaderContext.java
hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/ObserverContext.java
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/CompressionContext.java
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/ScannerContext.java
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/compactions/CompactionContext.java
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/NoLimitScannerContext.java
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFlushContext.java
hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/RpcCallContext.java
hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/RpcSchedulerContext.java
There was a problem hiding this comment.
thanks for sharing a list of *Info and *Context, I browsed few of them and seems Context should be still okie. so let's keep it with StoreContext and made this change simple, and I add the immutable keyword in the block of the class comment.
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
saintstack
requested changes
Jan 6, 2021
| public void setCompression(Compression.Algorithm compressAlgo) { | ||
| this.compressAlgo = compressAlgo; | ||
| } | ||
|
|
There was a problem hiding this comment.
You haven't uploaded new PR so will leave this as unresovled for now.
…ation in constructor of HStore
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
saintstack
reviewed
Jan 6, 2021
|
|
||
| // set block storage policy for store directory | ||
| String policyName = family.getStoragePolicy(); | ||
| if (null == policyName) { | ||
| policyName = this.conf.get(BLOCK_STORAGE_POLICY_KEY, DEFAULT_BLOCK_STORAGE_POLICY); | ||
| } | ||
| this.fs.setStoragePolicy(family.getNameAsString(), policyName.trim()); | ||
| getRegionFileSystem().setStoragePolicy(getColumnFamilyName(), policyName.trim()); |
There was a problem hiding this comment.
You were going to change these back?
There was a problem hiding this comment.
changed back to use family and region.getRegionFileSystem() directly.
| @@ -308,7 +294,7 @@ protected HStore(final HRegion region, final ColumnFamilyDescriptor family, | |||
| this.compactionCheckMultiplier = DEFAULT_COMPACTCHECKER_INTERVAL_MULTIPLIER; | |||
| } | |||
|
|
|||
| this.storeEngine = createStoreEngine(this, this.conf, this.comparator); | |||
| this.storeEngine = createStoreEngine(this, this.conf, getComparator()); | |||
There was a problem hiding this comment.
changed to use region.getCellComparator().
| @@ -46,6 +46,8 @@ | |||
| int PRIORITY_USER = 1; | |||
| int NO_PRIORITY = Integer.MIN_VALUE; | |||
|
|
|||
| int getBlockSize(); | |||
There was a problem hiding this comment.
Is this necessary? Its available on the ColumnFamilyDescriptor. Do we need to add new method on Store?
Was this change always here or did it just show up in recent amendments to PRs (I don't remember seeing it before but probably just me).
Thanks.
There was a problem hiding this comment.
this was new change in the latest commit of this PR, mainly it's used for HMobStore when calling createWriterInTmp. but let me change it using getStoreContext().getBlockSize() to avoid adding a new interface.
There was a problem hiding this comment.
Yes. Please. Then I'll +1 this nice PR. Thanks.
|
Just a few nits.... |
saintstack
approved these changes
Jan 6, 2021
|
🎊 +1 overall
This message was automatically generated. |
z-york
approved these changes
Jan 6, 2021
|
🎊 +1 overall
This message was automatically generated. |
taklwu
commented
Jan 6, 2021
Comment on lines
+115
to
+118
| @Override | ||
| public long heapSize() { | ||
| return FIXED_OVERHEAD; | ||
| } |
There was a problem hiding this comment.
@apurtell ping again on the heapSize() and implements HeapSize for StoreContext class, could you have another look? I should have fixed it, but will wait for your approval/resolve in 1 day or 2 day before merging it.
There was a problem hiding this comment.
@apurtell sorry for pinging again, I will merge this evening if I don't hear from you
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
This was referenced Dec 27, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adding StoreContext which contains the metadata about the HStore. This
meta data can be used across the HFileWriter/Readers and other HStore
consumers without the need of passing around the complete store and
exposing its internals.